Compute Legendre symbol for `hash_to_curve` by davidnevadoc · Pull Request #77 · privacy-ethereum/halo2curves

davidnevadoc · 2023-08-05T20:25:42Z

This is an attempt to close #73 .

Unlike in Arkworks, I believe there is no clean way to access the modulus p of a PrimeField here. I have gotten it using:

    let modulus: BigUint =
        BigUint::from_str_radix(F::MODULUS.strip_prefix("0x").unwrap(), 16).unwrap();

although in the docs it is explicitly mentioned that:

The encoding of the modulus is implementation-specific. Generic users of the PrimeField trait should treat this string as opaque.

Suggestions for alternative approaches are welcome.

Update: MODULUS - 1 value is now obtained by getting the representation of -F::ONE so the method above can be avoided. (Thanks to @huitseeker for the suggestion).

huitseeker · 2023-08-08T02:16:41Z

Could F::ZERO - F::ONE work (as value for MODULUS - 1)?

davidnevadoc · 2023-08-08T10:01:41Z

Thanks for the suggestion @huitseeker! Updated in 3b7e8b3

huitseeker

It would be great to see a couple of tests. One curve-independent thing to test being the relationship between legendre and foo.sqrt().is_some().

Another thought it this is pinned on receiving p_minus_one, which is systematically divided by two during the computation. Could we possibly require the field definition to include a constant for (p-1)/2?

han0110

LGTM, also think it'd be nice to have (p-1)/2 as constant somewhere (perhaps some private helper trait).

davidnevadoc · 2023-08-11T09:52:21Z

Thanks for the reviews! I have added a couple of tests in 0b40c37

I agree with the comments regarding fixing (p-1)/2, I will experiment a bit and give it a shot in a separate PR.

CPerezz

Just one suggestion that not sure makes sense. Would like to at least discuss it.

CPerezz · 2023-08-11T15:17:17Z

+#[inline]
+pub(crate) fn legendre<F: PrimeField>(elem: F, p_minus_one: BigUint) -> F {
+    let exp: BigUint = p_minus_one >> 1;
+    elem.pow(exp.to_u64_digits())
+}


Can't we pass P-1 to the macro invocation/function so that is treated as an asociated constant of F or similar? SO that we write this fn as a macro instead of a fn hence we get the constant for p_minus_one directly.

Another idea is to have a F::MINUS_ONE_AS_BIGUNT for each curve field. Being that implemented via any of the macros.
Then implement all of the legendre stuff inside of a macro for each of them having minus_one as ctant too.

I'd at least bench the difference and see if it makes sense.

CPerezz · 2023-08-11T15:17:25Z

+pub(crate) fn is_quadratic_non_residue<F: PrimeField>(e: F, p_minus_one: BigUint) -> Choice {
+    legendre(e, p_minus_one).ct_eq(&-F::ONE)
+}


CPerezz · 2023-08-11T15:18:15Z

+#[inline]
+pub(crate) fn mod_minus_one<F: PrimeField>() -> BigUint {
+    BigUint::from_bytes_le((-F::ONE).to_repr().as_ref())
+}


This will not need to exist if we do what I suggested above. Or at least will only be useful at compile time in a lazy_static block or similar.

CPerezz · 2023-08-11T15:19:27Z

    let gx2 = gx2 + b;
    // 21. e2 = is_square(gx2) AND NOT e1    # Avoid short-circuit logic ops
-    let e2 = gx2.sqrt().is_some() & (!e1);
+    let e2 = !is_quadratic_non_residue(gx2, p_minus_one) & (!e1);


p_minus_one could be a constant.

davidnevadoc · 2023-08-11T20:09:04Z

I have tried to address the issue of pre-computing and passing around (p-1)/2 in a different PR #79.
Let me know if we can merge this PR first and add this improvement later. Thanks everybody for the feedback! :)

CPerezz · 2023-08-14T09:11:22Z

I have tried to address the issue of pre-computing and passing around (p-1)/2 in a different PR #79. Let me know if we can merge this PR first and add this improvement later. Thanks everybody for the feedback! :)

We are at a point that merging without benchmarks feels like an error. I don't want to land things in the codebase for which we don't know the performance implications.
So, unless we get some benchmarks, I suggest to merge the fix/upgrade of #79 here and then merge this once we have the measurements.

davidnevadoc · 2023-08-17T18:11:53Z

I have added the changes from #79 here and I've also added some benchmarks as @CPerezz requested. There is practically no change in the performance of hash to curve:

   Finished bench [optimized] target(s) in 23.15s
     Running benches/hash_to_curve.rs (target/release/deps/hash_to_curve-c2c106ce25b99f82)
Gnuplot not found, using plotters backend
Hash to Secp256k1       time:   [147.12 µs 147.22 µs 147.35 µs]
                        change: [-0.1760% +0.0373% +0.2060%] (p = 0.73 > 0.05)
                        No change in performance detected.
Found 9 outliers among 100 measurements (9.00%)
  4 (4.00%) high mild
  5 (5.00%) high severe

Hash to Bn256           time:   [153.73 µs 153.81 µs 153.89 µs]
                        change: [-1.5987% -1.2515% -0.9976%] (p = 0.00 < 0.05)
                        Change within noise threshold.
Found 6 outliers among 100 measurements (6.00%)
  1 (1.00%) high mild
  5 (5.00%) high severe

han0110

LGTM! Some nitpicks that could be ignored.

- Add Legendre macro with norm and legendre symbol computation - Add macro for automatic implementation in prime fields

* Add field conversion to/from `[u64;4]` (privacy-ethereum#80) * feat: add field conversion to/from `[u64;4]` * Added conversion tests * Added `montgomery_reduce_short` for no-asm * For bn256, uses assembly conversion when asm feature is on * fix: remove conflict for asm * chore: bump rust-toolchain to 1.67.0 * Compute Legendre symbol for `hash_to_curve` (privacy-ethereum#77) * Add `Legendre` trait and macro - Add Legendre macro with norm and legendre symbol computation - Add macro for automatic implementation in prime fields * Add legendre macro call for prime fields * Remove unused imports * Remove leftover * Add `is_quadratic_non_residue` for hash_to_curve * Add `legendre` function * Compute modulus separately * Substitute division for shift * Update modulus computation * Add quadratic residue check func * Add quadratic residue tests * Add hash_to_curve bench * Implement Legendre trait for all curves * Move misplaced comment * Add all curves to hash bench * fix: add suggestion for legendre_exp * fix: imports after rebase * Add simplified SWU method (privacy-ethereum#81) * Fix broken link * Add simple SWU algorithm * Add simplified SWU hash_to_curve for secp256r1 * add: sswu z reference * update MAP_ID identifier Co-authored-by: Han <tinghan0110@gmail.com> --------- Co-authored-by: Han <tinghan0110@gmail.com> * Bring back curve algorithms for `a = 0` (privacy-ethereum#82) * refactor: bring back curve algorithms for `a = 0` * fix: clippy warning * fix: Improve serialization for prime fields (privacy-ethereum#85) * fix: Improve serialization for prime fields Summary: 256-bit field serialization is currently 4x u64, ie. the native format. This implements the standard of byte-serialization (corresponding to the PrimeField::{to,from}_repr), and an hex-encoded variant of that for (de)serializers that are human-readable (concretely, json). - Added a new macro `serialize_deserialize_32_byte_primefield!` for custom serialization and deserialization of 32-byte prime field in different struct (Fq, Fp, Fr) across the secp256r, bn256, and derive libraries. - Implemented the new macro for serialization and deserialization in various structs, replacing the previous `serde::{Deserialize, Serialize}` direct use. - Enhanced error checking in the custom serialization methods to ensure valid field elements. - Updated the test function in the tests/field.rs file to include JSON serialization and deserialization tests for object integrity checking. * fixup! fix: Improve serialization for prime fields --------- Co-authored-by: Carlos Pérez <37264926+CPerezz@users.noreply.github.com> * refactor: (De)Serialization of points using `GroupEncoding` (privacy-ethereum#88) * refactor: implement (De)Serialization of points using the `GroupEncoding` trait - Updated curve point (de)serialization logic from the internal representation to the representation offered by the implementation of the `GroupEncoding` trait. * fix: add explicit json serde tests * Insert MSM and FFT code and their benchmarks. (privacy-ethereum#86) * Insert MSM and FFT code and their benchmarks. Resolves taikoxyz/zkevm-circuits#150. * feedback * Add instructions * feeback * Implement feedback: Actually supply the correct arguments to `best_multiexp`. Split into `singlecore` and `multicore` benchmarks so Criterion's result caching and comparison over multiple runs makes sense. Rewrite point and scalar generation. * Use slicing and parallelism to to decrease running time. Laptop measurements: k=22: 109 sec k=16: 1 sec * Refactor msm * Refactor fft * Update module comments * Fix formatting * Implement suggestion for fixing CI --------- Co-authored-by: David Nevado <davidnevadoc@users.noreply.github.com> Co-authored-by: Han <tinghan0110@gmail.com> Co-authored-by: François Garillot <4142+huitseeker@users.noreply.github.com> Co-authored-by: Carlos Pérez <37264926+CPerezz@users.noreply.github.com> Co-authored-by: einar-taiko <126954546+einar-taiko@users.noreply.github.com>

* Add field conversion to/from `[u64;4]` (privacy-ethereum#80) * feat: add field conversion to/from `[u64;4]` * Added conversion tests * Added `montgomery_reduce_short` for no-asm * For bn256, uses assembly conversion when asm feature is on * fix: remove conflict for asm * chore: bump rust-toolchain to 1.67.0 * Compute Legendre symbol for `hash_to_curve` (privacy-ethereum#77) * Add `Legendre` trait and macro - Add Legendre macro with norm and legendre symbol computation - Add macro for automatic implementation in prime fields * Add legendre macro call for prime fields * Remove unused imports * Remove leftover * Add `is_quadratic_non_residue` for hash_to_curve * Add `legendre` function * Compute modulus separately * Substitute division for shift * Update modulus computation * Add quadratic residue check func * Add quadratic residue tests * Add hash_to_curve bench * Implement Legendre trait for all curves * Move misplaced comment * Add all curves to hash bench * fix: add suggestion for legendre_exp * fix: imports after rebase * Add simplified SWU method (privacy-ethereum#81) * Fix broken link * Add simple SWU algorithm * Add simplified SWU hash_to_curve for secp256r1 * add: sswu z reference * update MAP_ID identifier Co-authored-by: Han <tinghan0110@gmail.com> --------- Co-authored-by: Han <tinghan0110@gmail.com> * Bring back curve algorithms for `a = 0` (privacy-ethereum#82) * refactor: bring back curve algorithms for `a = 0` * fix: clippy warning * fix: Improve serialization for prime fields (privacy-ethereum#85) * fix: Improve serialization for prime fields Summary: 256-bit field serialization is currently 4x u64, ie. the native format. This implements the standard of byte-serialization (corresponding to the PrimeField::{to,from}_repr), and an hex-encoded variant of that for (de)serializers that are human-readable (concretely, json). - Added a new macro `serialize_deserialize_32_byte_primefield!` for custom serialization and deserialization of 32-byte prime field in different struct (Fq, Fp, Fr) across the secp256r, bn256, and derive libraries. - Implemented the new macro for serialization and deserialization in various structs, replacing the previous `serde::{Deserialize, Serialize}` direct use. - Enhanced error checking in the custom serialization methods to ensure valid field elements. - Updated the test function in the tests/field.rs file to include JSON serialization and deserialization tests for object integrity checking. * fixup! fix: Improve serialization for prime fields --------- Co-authored-by: Carlos Pérez <37264926+CPerezz@users.noreply.github.com> * refactor: (De)Serialization of points using `GroupEncoding` (privacy-ethereum#88) * refactor: implement (De)Serialization of points using the `GroupEncoding` trait - Updated curve point (de)serialization logic from the internal representation to the representation offered by the implementation of the `GroupEncoding` trait. * fix: add explicit json serde tests * Insert MSM and FFT code and their benchmarks. (privacy-ethereum#86) * Insert MSM and FFT code and their benchmarks. Resolves taikoxyz/zkevm-circuits#150. * feedback * Add instructions * feeback * Implement feedback: Actually supply the correct arguments to `best_multiexp`. Split into `singlecore` and `multicore` benchmarks so Criterion's result caching and comparison over multiple runs makes sense. Rewrite point and scalar generation. * Use slicing and parallelism to to decrease running time. Laptop measurements: k=22: 109 sec k=16: 1 sec * Refactor msm * Refactor fft * Update module comments * Fix formatting * Implement suggestion for fixing CI * Re-export also mod `pairing` and remove flag `reexport` to alwasy re-export (privacy-ethereum#93) fix: re-export also mod `pairing` and remove flag `reexport` to alwasy re-export * fix regression in privacy-ethereum#93 reexport field benches aren't run (privacy-ethereum#94) fix regression in privacy-ethereum#93, field benches aren't run * Fast modular inverse - 9.4x acceleration (privacy-ethereum#83) * Bernstein yang modular multiplicative inverter (#2) * rename similar to privacy-ethereum#95 --------- Co-authored-by: Aleksei Vambol <77882392+AlekseiVambol@users.noreply.github.com> * Fast isSquare / Legendre symbol / Jacobi symbol - 16.8x acceleration (privacy-ethereum#95) * Derivatives of the Pornin's method (taikoxyz#3) * renaming file * make cargo fmt happy * clarifications from privacy-ethereum#95 (comment) [skip ci] * Formatting and slightly changing a comment --------- Co-authored-by: Aleksei Vambol <77882392+AlekseiVambol@users.noreply.github.com> * chore: delete bernsteinyang module (replaced by ff_inverse) * Bump version to 0.4.1 --------- Co-authored-by: David Nevado <davidnevadoc@users.noreply.github.com> Co-authored-by: Han <tinghan0110@gmail.com> Co-authored-by: François Garillot <4142+huitseeker@users.noreply.github.com> Co-authored-by: Carlos Pérez <37264926+CPerezz@users.noreply.github.com> Co-authored-by: einar-taiko <126954546+einar-taiko@users.noreply.github.com> Co-authored-by: Mamy Ratsimbazafy <mamy_github@numforge.co> Co-authored-by: Aleksei Vambol <77882392+AlekseiVambol@users.noreply.github.com>

davidnevadoc requested review from CPerezz and han0110 August 5, 2023 21:05

davidnevadoc requested a review from kilic August 9, 2023 20:48

huitseeker reviewed Aug 9, 2023

View reviewed changes

han0110 approved these changes Aug 11, 2023

View reviewed changes

CPerezz reviewed Aug 11, 2023

View reviewed changes

davidnevadoc force-pushed the feat/legendre branch from 0b40c37 to 6bea1fe Compare August 17, 2023 18:09

davidnevadoc mentioned this pull request Aug 18, 2023

[WIP] Add Legendre trait #79

Closed

davidnevadoc requested review from han0110 and huitseeker August 18, 2023 18:54

han0110 approved these changes Aug 21, 2023

View reviewed changes

Comment thread src/hash_to_curve.rs

Comment thread src/hash_to_curve.rs

davidnevadoc added 13 commits August 22, 2023 01:29

Add Legendre trait and macro

b6b6216

- Add Legendre macro with norm and legendre symbol computation - Add macro for automatic implementation in prime fields

Add legendre macro call for prime fields

d9052ac

Remove unused imports

f6ffe4f

Remove leftover

963f357

Add is_quadratic_non_residue for hash_to_curve

a2e764c

Add legendre function

fc513a4

Compute modulus separately

04ad73d

Substitute division for shift

6b2777f

Update modulus computation

fd4a7c0

Add quadratic residue check func

6a22a7e

Add quadratic residue tests

8ed6266

Add hash_to_curve bench

f5219a4

Implement Legendre trait for all curves

3832485

davidnevadoc added 3 commits August 22, 2023 01:32

Move misplaced comment

ed460ec

Add all curves to hash bench

78a56d5

fix: add suggestion for legendre_exp

bdbb2f0

davidnevadoc force-pushed the feat/legendre branch 2 times, most recently from c2af9ed to 5e7022c Compare August 22, 2023 09:47

fix: imports after rebase

5e7022c

davidnevadoc added this pull request to the merge queue Aug 22, 2023

Merged via the queue into main with commit 1d71d34 Aug 22, 2023

mratsim mentioned this pull request Aug 29, 2023

Fast modular inverse - 9.4x acceleration #83

Merged

CPerezz deleted the feat/legendre branch September 5, 2023 12:45

mratsim mentioned this pull request Oct 13, 2023

Fast isSquare / Legendre symbol / Jacobi symbol - 16.8x acceleration #95

Merged

CPerezz mentioned this pull request Dec 18, 2023

chore: Bump to 0.5.0 for release #114

Merged

Conversation

davidnevadoc commented Aug 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

huitseeker commented Aug 8, 2023

Uh oh!

davidnevadoc commented Aug 8, 2023

Uh oh!

huitseeker left a comment

Choose a reason for hiding this comment

Uh oh!

han0110 left a comment

Choose a reason for hiding this comment

Uh oh!

davidnevadoc commented Aug 11, 2023

Uh oh!

CPerezz left a comment

Choose a reason for hiding this comment

Uh oh!

CPerezz Aug 11, 2023

Choose a reason for hiding this comment

Uh oh!

CPerezz Aug 11, 2023

Choose a reason for hiding this comment

Uh oh!

CPerezz Aug 11, 2023

Choose a reason for hiding this comment

Uh oh!

CPerezz Aug 11, 2023

Choose a reason for hiding this comment

Uh oh!

davidnevadoc commented Aug 11, 2023

Uh oh!

CPerezz commented Aug 14, 2023

Uh oh!

davidnevadoc commented Aug 17, 2023

Uh oh!

han0110 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

davidnevadoc commented Aug 5, 2023 •

edited

Loading